Add COWPtr and its unittest #7240
Conversation
reyoung
force-pushed
the
feature/make_lod_a_share_ptr
branch
2 times, most recently
from
aa4a6aa to
d2a521e
Compare
It will be used for LoD information in LoDTensor since LoD is a copy on write field. It is pretty slow for copying LoD information between operators. For resnet it will cost roughly 10% time of whole time, including reading data.
reyoung
force-pushed
the
feature/make_lod_a_share_ptr
branch
from
d2a521e to
0cfb546
Compare
paddle/framework/details/cow_ptr.h
Outdated
| ThreadUnsafeOwnershipFlags(const ThreadUnsafeOwnershipFlags& o) = delete; | ||
| ThreadUnsafeOwnershipFlags& operator=(const ThreadUnsafeOwnershipFlags& o) = | ||
| delete; | ||
| ThreadUnsafeOwnershipFlags(ThreadUnsafeOwnershipFlags&& o) = default; |
There was a problem hiding this comment.
Use the Maroc DISABLE_COPY_AND_ASSIGN in https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/platform/macros.h#L19
There was a problem hiding this comment.
They are not exactly same. OwnershipFlag just disable copy constructor, but enable move constructor.
paddle/framework/details/cow_ptr.h
Outdated
| bool flag_; | ||
| }; | ||
|
|
||
| // Copy On Write pointer. |
There was a problem hiding this comment.
Copy On Write -> Copy-On-Write https://en.wikipedia.org/wiki/Copy-on-write
paddle/framework/details/cow_ptr.h
Outdated
| explicit COWPtr(T* ptr) : payload_(ptr), ownership_{true} {} | ||
|
|
||
| // Move methods. Steal ownership from origin | ||
| COWPtr(COWPtr&& o) |
There was a problem hiding this comment.
What does the o mean? owner? a more meaningful name is needed.
There was a problem hiding this comment.
Done.
o means other
|
|
||
| private: | ||
| std::shared_ptr<T> payload_; | ||
| OwnershipFlags ownership_; |
There was a problem hiding this comment.
Please add comments for payload_ and ownership_.
paddle/framework/details/cow_ptr.h
Outdated
| void Reset() { | ||
| ownership_.AcquireOwnershipOnce([this] { payload_.reset(); }); | ||
| payload_.reset(new T()); | ||
| } |
There was a problem hiding this comment.
I can understand the AcquireOwnershipOnce, MutableData, Reset functions from the unit test. Maybe better to add comments for these function.
There was a problem hiding this comment.
Done.
Also, remove the Reset method. Reset is not necessary because we can create a new COWPtr, and assign it to origin COWPtr. The effect is same as Reset.
2 participants
It will be used for LoD information in LoDTensor since LoD is a copy
on write field.
It is pretty slow for copying LoD information between operators. For
resnet it will cost roughly 10% time of whole time, including reading
data.
Related issue #7239